fix: Use foreach/doSNOW for restart write.ensemble.configs to restore logger output#4018
fix: Use foreach/doSNOW for restart write.ensemble.configs to restore logger output#4018S1DDHEY wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Sorry to jump in after approval, but this looks to me like it will make sda.enkf.multisite stop respecting the run_parallel flag and instead use n-1 cores no matter what the user requests. @DongchenZ am I missing another place the parallel setting gets passed around?
|
@infotroph sorry for the late reply, this patch would make restart path ignore the |
Good catch. If it's easy to fix in this PR go for it, but not required. But more importantly for this PR, how sure are you that this actually fixes the issue? I don't see either approach printing to the console: Note that no logger output is shown in either case. You mentioned using |
|
@infotroph After digging a bit more, I think you’re right, switching from A probable fix seems like it would be to stop relying on worker console output entirely and instead return log messages from each worker as part of the task result, then print them in the parent process after collection. In other words, have the worker return something like It wouldn’t be truly live streaming, but it should make the messages reliably visible in normal SDA runs and is a lot easier to reason about and test. |
Description
Fixes #3581
Replaces the restart-path parallel dispatch for
write.ensemble.configs()insda.enkf_MultiSite.Rfromfurrr::future_map2()toforeach+doSNOW, matching the existing non-restart code path.This fixes missing/delayed
PEcAn.loggeroutputTesting
write.ensemble.configs()is emitted by reproducing the “multi-path input with no<samplingspace>” error underforeach/doSNOW(captured viaPEcAn.logger::logger.setOutputFile()from the worker).